fix(item-update): visibility of range spanning items not always correct #1846
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove binary search for items having an end time because items starting before range and ending after it were left out. Fixes visjs#1657
ChristianKrebel
commented
Dec 12, 2024
| @@ -813,7 +813,7 @@ class Group { | |||
| */ | |||
| _resetSubgroups() { | |||
| for (const subgroup in this.subgroups) { | |||
| if (this.subgroups.hasOwnProperty(subgroup)) { | |||
| if (Object.prototype.hasOwnProperty.call(this.subgroups, subgroup)) { | |||
There was a problem hiding this comment.
This is not related to the fix but the pre-commit-hook did not let me commit without fixing it because it lints the whole file instead of only the changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Items starting before the visible range and ending after it were not always shown if zoomed in too much because of the binary search used for it. I tried to fix the conditions but I could not get it to work with a binary search (maybe someone has a better way?). Also, different approaches like spatial index logic fixed the problem but did not improve performance. So I kept it simple and removed the binary search case for items that have an end time and always brute force check them for visibility. This may worsen performance when having many items with end times but in my manual testing (private repo) it is not performant either way.
Question: Should we make this fix optional per config because it may have a bigger impact?
Module: timeline/component/group
Fixes #1657